gh-154916: Fix data race in ga_iter_reduce under free-threading - #154944
gh-154916: Fix data race in ga_iter_reduce under free-threading#154944tekinertekin wants to merge 2 commits into
Conversation
ga_iternext takes gi->obj out with an atomic exchange and then drops the reference, while ga_iter_reduce read the same field twice without synchronisation. The two reads can straddle the exchange, so the guard can observe a non-NULL pointer that is then passed to Py_BuildValue after the owning thread has already released it. Take a single strong reference instead, and mark the stored object as maybe-weakref in ga_iter, which _Py_XGetRef requires of the writer.
|
All commit authors signed the Contributor License Agreement. |
|
This PR ha been clearly just generated by an agent. I believe this approach is too complicate and should not be done like that. Having a critical section may be enough. |
|
@tekinertekin Should you continue with fully-LLM generated PRs, we will restrict the access to our repositories as it goes against our AI policy. We don't forbid users to use LLMs, but we don't want them to just ask an agent to do it and claim the contribution afterwards. |
First of all, thank you very much for your interest. I misunderstood a statement in the policy text(my fault) and took these actions. I apologize for taking up your time. I promise to be more careful in my future contributions. Please forgive me for disturbing you. I have read and understood your warnings and will implement them. Thank you again for your comments. You can be sure that I will be more careful next time. |
2 participants
Issue: #154916
ga_iternexttakesgi->objout with_Py_atomic_exchange_ptrand then drops thereference, while
ga_iter_reducereads the same field twice without synchronisation:The two reads can straddle that exchange, so the guard can observe a non-NULL
pointer that is then passed to
Py_BuildValueafter the owning thread hasalready called
Py_DECREFon it. That makes this a potential use-after-freerather than only a torn read. ThreadSanitizer trips on the guard read first,
which is what the issue reports.
The fix takes a single strong reference instead:
_Py_XGetRefin the free-threaded build, plainPy_XNewRefotherwise, with"N(N)"soPy_BuildValueconsumes the reference we now own._Py_XGetRefdocuments that the writer must set maybe-weakref on the storedobject, otherwise its try-incref can never succeed from another thread and it
spins.
ga_iterstores with a plainPy_NewRef, so the writer side needs_PyObject_SetMaybeWeakref.Racing with
next()may legitimately observe either the object or the exhaustediterator; both reductions are correct and that does not change here.
Verification
Built on arm64 macOS with
--disable-gil --with-thread-sanitizer --with-pydebug.Reproducer: 4 threads calling
next()and 4 calling__reduce__()on one sharediter(list[int]).ga_iternext:946↔ga_iter_reduce:1000, both orderingstest_genericalias,test_types,test_typing: 913 tests pass.test_genericalias test_types -R 3:3: no leaks. TheO→Nchange movesownership of the reference, so this seemed worth checking explicitly.
__reduce__output and pickle round-trip are unchanged for both a live and anexhausted iterator.
One open question
I kept this lock-free to stay symmetric with
ga_iternextas gh-154108 left it.The alternative is the
list_item_implpattern — a critical section plus_Py_NewRefWithLock— but that only synchronises ifga_iternexttakes the locktoo, which would mean revising the design just merged in gh-154108.
_Py_XGetRefhas no other call site in the tree, so if you would rather have thecritical-section version, I am happy to redo it that way.
types.GenericAliasiterator inga_iter_reduceunder free-threading (follow-up to gh-154043) #154916